Multi-agent review

Four specialist agents (oauth/security, Go correctness/style, test quality, general/API) reviewed this PR. Codex cross-review was skipped (codex CLI not installed locally). 0 HIGH, 5 MEDIUM, 7 LOW findings survived consensus scoring. None are blocking; this is posted as COMMENT.

Executive summary

A focused follow-up to stacked PR #5348 closing deferred review comments C3/C4. It threads ScopesSupported and BaselineClientScopes into the CIMD storage decorator so CIMD documents are validated against the same policy as DCR (via registration.ValidateScopes), and rejects unsupported grant_types / response_types at fetch time. As supporting refactor, unionScopes is promoted to registration.UnionScopes. Approach is correct in shape: reuse a single validator, reject earlier in the pipeline, surface a startup warning for the CIMD+baseline interaction. Most surviving findings are correctness-leaning details and test-quality nits.

Pros

• Reuses registration.ValidateScopes instead of duplicating policy.
• Rejects invalid CIMD documents at fetch time (not at /authorize), giving operators a clear, early error.
• Error class change ErrNotFound → ErrInvalidClient for "fetched but policy-violating" is semantically correct per RFC 6749 §5.2.
• Public/PKCE invariant preserved (Public: true unconditional; allowlists still exclude client_credentials).
• slog.Warn at startup surfaces the non-obvious CIMD+baseline interaction.
• Doc comments explain the design intent and the test-only nil escape hatch.
• PR title, body, and size comply with .claude/rules/pr-creation.md.

Cons

• The "same scope policy as DCR" claim has one silent divergence: when doc.Scope == "" and ScopesSupported ⊃ DefaultScopes, the CIMD client receives the full ScopesSupported while a DCR client in the same position receives only DefaultScopes. See F5.
• NewCIMDStorageDecorator now takes six positional args with two adjacent same-typed []string slices — easy to swap silently.
• TestUnionScopes was not relocated alongside the moved function and still lives in pkg/authserver/server/handlers/, where scopes.go is now empty.
• New rejection tests assert require.Error(...) only — they would still pass if the code regressed to fosite.ErrNotFound, defeating the PR's main intent.

Consensus findings table

Detailed comments are inline on the relevant changed lines.

How to test

git fetch origin pull/5385/head:pr-5385 && git checkout pr-5385
task lint-fix
task test

Plus targeted checks against an embedded AS:

1. C3 — declared scope outside ScopesSupported: serve a CIMD doc declaring scope: "openid profile" with scopes_supported: ["openid"]. Expect fosite.ErrInvalidClient.
2. C3 — omitted scope: same setup but doc omits scope. Confirm the client carries exactly ["openid"], not DefaultScopes (and consider what should happen when ScopesSupported ⊃ DefaultScopes — see F5).
3. C4 — unsupported grant_type: serve a doc with grant_types: ["client_credentials"]. Expect rejection.
4. C4 — missing authorization_code: serve a doc with grant_types: ["refresh_token"] only. Expect rejection.
5. C4 — unsupported response_type: serve a doc with response_types: ["token"]. Expect rejection.
6. slog.Warn: start the AS with cimd.enabled: true and baseline_client_scopes: ["offline_access"]. Expect one WARN line at startup.
7. Load-bearing-test check: temporarily revert if !allowedCIMDGrantTypes[gt] to always-false, re-run go test ./pkg/authserver/storage/.... TestFetch_RejectsUnsupportedGrantType should fail; revert when done.

Generated with Claude Code (multi-agent /dev:pr-review).

Re-review LGTM — all 12 findings from the prior multi-agent review are resolved. F3 went from 11 separate tests to 3 table-driven tests (plus a serveCIMDDocWithFields helper), F5 chose the fail-closed option (require explicit scope when DefaultScopes ⊄ ScopesSupported), and F9 extracted ValidatePublicGrantTypes / ValidatePublicResponseTypes so DCR and CIMD now share a single validation source. The errors.Is(err, fosite.ErrInvalidClient) assertions pin the error-type change so a regression to ErrNotFound would fail tests.

One nice-to-have follow-up (not blocking)

defaultCIMDGrantTypes and defaultCIMDResponseTypes in cimd_decorator.go are still parallel to defaultGrantTypes / defaultResponseTypes in the registration package. After F9 the validation is shared, but the default application is still duplicated — if someone later widens registration.defaultGrantTypes for DCR (e.g., adds a new public-client grant), they'd need to remember to update the CIMD copy too, and task lint won't catch the mismatch.

Since ValidatePublicGrantTypes / ValidatePublicResponseTypes already return the defaulted slice, buildFositeClient could use those return values and the CIMD-side constants could be deleted. Closes the last DCR/CIMD parity gap. Not blocking for this PR — could be a small follow-up.

Approving.